feat(store): billing subscription and checkout reads skip soft-deleted rows - #1963
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: raystack/frontier/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughBilling checkout and subscription read queries now use ChangesBilling repository live reads
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to Billing reads now exclude soft-deleted subscriptions and checkouts. No concrete merge-blocking issue is established; the change is mergeable subject to normal checks. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 36848031156Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.5%) to 54.262%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions88 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
ec2e29d to
d568781
Compare
End to end check on a local sandboxThis branch is rebased on This branch: 31 of 31 pass. New in this PR: subscriptions and checkouts
The three controls pass on both builds, so live rows behave the same. The other six fail on Regression check: the 22 scenarios from #1962 (all pass on both builds)
These pass on How the data was set up:
Not covered: anything that writes |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/store/postgres/billing_checkout_repository.go (1)
239-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
GetByNamequeries anamecolumn that the table does not have.The PR description says
billing_checkoutshas nonamecolumn. This method fails with a DB error on every call. ThefromLivechange does not alter that. The PR description says the method is unreachable, so the risk is low. Remove the method, or add a comment that marks it as dead code.internal/store/postgres/billing_subscription_repository.go (1)
268-268: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
GetByNamestays broken, but this change does not make it worse.The
billing_subscriptionstable has nonamecolumn, according to the PR description. The query fails with a database error before the newfromLivefilter has any effect. The PR notes this method is unreachable. Consider removing it or tracking it in a follow-up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: raystack/frontier/.coderabbit.yaml
Review profile: CHILL
Plan: Advanced
Run ID: 655282f2-5f7b-42ef-948f-de47f78dbc64
📒 Files selected for processing (4)
internal/store/postgres/billing_checkout_repository.gointernal/store/postgres/billing_checkout_repository_pg_test.gointernal/store/postgres/billing_subscription_repository.gointernal/store/postgres/billing_subscription_repository_pg_test.go
Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.
AmanGIT07
left a comment
There was a problem hiding this comment.
The org delete reads subscriptions and checkouts through these filtered methods (core/deleter/service.go:407, :418), then hard-deletes. Once a row has deleted_at, the subscription is left behind and blocks the customer delete on the foreign key. The checkout is removed without its audit record. Nothing sets deleted_at yet, so this is fine today. Can we add a TODO(fix) next to the one at :410, so the PR that makes this delete soft picks it up?
…e from the org delete
|
Thanks for the review. Two follow-ups are pushed.
|
Summary
The billing subscription and checkout repositories read rows without checking
deleted_at. They now skip deleted rows, using the sameliveandfromLivehelpers as the rest of the store.These are the last two of the five repositories the org delete cascade touches. #1962 did the other three and has merged, so with this change all five filter. Nothing writes
deleted_atto either table yet, so every read returns the same rows as before.Changes
GetByID,GetByName,GetByProviderIDandListnow read fromfromLive.GetByID,GetByNameandListnow read fromfromLive.Technical Details
The two correlated subqueries on
billing_customersin the subscription repository are left alone on purpose. They only fill the org id and customer name into theRETURNINGclause ofCreateandUpdateByID, for the audit record. Filtering them would do more than blank two columns: both fields are plain strings, so a NULL would fail the struct scan and abort the whole write. An audit record should say which org a subscription belonged to even after the customer is deleted.GetByNameon both repositories is unreachable. Neither table has anamecolumn and neither method has a caller, so calling either fails with a postgres error beforedeleted_atmatters. I filtered them to keep the files consistent, but they are dead code and deleting them would be a fair follow-up.The update paths and the hard deletes are untouched, matching #1962.
No migration. Subscriptions have carried
deleted_atsince the table was created, and checkouts got it in20260916100000_soft_delete_columns.Test Plan
go test -run 'TestBillingSubscriptionRepositoryPG|TestBillingCheckoutRepositoryPG' ./internal/store/postgres/passesmaingolangci-lint run ./internal/store/postgres/...reports no issuesgo test ./internal/store/postgres/ ./billing/...passesEach suite seeds a live row and a deleted one, and I watched all five tests fail before adding the filters. Reverting the seven
fromLivecalls was also checked: every test fails, and each test exercises exactly one read, so each one catches its own change.SQL Safety (if your PR touches
*_repository.goorgoqu.*)?placeholders,goqu.Ex{}, orgoqu.Record{}— neverfmt.Sprintfor+building a query that gets executed.ToSQL()callers capture and forward params (query, params, err := stmt.ToSQL(); db.…Context(ctx, …, query, params...)). Neverquery, _, err := ….?placeholders inside single-quoted SQL literals ingoqu.L(usemake_interval(hours => ?)-style functions instead).//nolint:forbidigoor// #nosec G20xannotation has a one-line justification on the same line that a reviewer can verify.The added predicates come from the existing
live()helper and bind no values. The onlyfmt.Sprintfin the diff buildsTRUNCATEin the test teardowns from package table constants, the same as the other suites here.